Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.x] RLS without central user (role) having bypassrls #1288

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

JustinElst
Copy link

Original issue

Description

I am using a managed Postgres solution where the admin role does not have and can not get BYPASSRLS.
That leads to the behaviour that when using the 'central connection' i don't have access to tenant data (including 'tennant_user').
That makes the management of tenants impossible.

So I thought of a solution giving the tenancy.rls.user.username (database tenant_user) the rsl policies already implemented by the package.
AND giving the central user a policy with full access:

CREATE POLICY {$table}_central_rls_policy ON {$table} AS PERMISSIVE TO {$centralUserName} USING (true);

This worked for me :)

Why this should be added

I have implemented this in the CreateUserWithRLSPolicies command and 'TableRLSManager' class.
This could use some more refactoring and possibly detecting if central user has BYPASSRLS permissions and ommiting the central policy.
But this is to give you an idea of what I did with this.
What do you think?

Original @stancl reply:

Thanks for the detailed writeup! I have some plans for refactoring the RLS feature, so I think I could take a look at this alongside that. This seems like an implementation detail so there shouldn't be any breaking changes.

Would the ideal behavior be basically checking whether the central user has BYPASSRLS perms when creating the policies, and if it doesn't, append the policies with the AS PERMISSIVE TO ...?

Will just need to find the right spot for this, to not run the check multiple times ideally but also to make this work well with the hashing logic.

@JustinElst JustinElst changed the title RLS without central user (role) having bypassrls [4.x] RLS without central user (role) having bypassrls Jan 9, 2025
@stancl
Copy link
Member

stancl commented Jan 9, 2025

Thanks for the PR! Can you revert the formatting changes?

@stancl
Copy link
Member

stancl commented Jan 9, 2025

Ah nevermind, I see that there's an extra loop now.

@stancl
Copy link
Member

stancl commented Jan 9, 2025

Currently in the process of testing an alternative implementation for this btw, so no need to bother fixing phpstan for now.

@stancl
Copy link
Member

stancl commented Jan 9, 2025

Could you provide some concrete reproduction steps for the bug first? ALTER ROLE (central user) NOBYPASSRLS in our tests doesn't seem to do it.

@JustinElst
Copy link
Author

JustinElst commented Jan 10, 2025

The user that is used as the central user in tenancy should not have the superuser or bypassrls.

Could you try with:

ALTER USER *central user* NOBYPASSRLS

Or just create a new user:

create user *central user*
    createdb
    createrole;

Some possible steps to follow:

  1. create central_user (see above)
  2. setup database grants grant connect, create, temporary on database *yourdatabase* to *central user*;
  3. setup schema grants grant create, usage on schema *yourschema* to *central user*;
  4. setup tenancy with that user
  5. run 'artisan tenants:rls'
  6. check if central user can see all resources

Without this pull request the app will throw a error or just not select the data, because the user cannot access the data without rls bypass.

You can check the policies by running select * from pg_policies.

@stancl
Copy link
Member

stancl commented Jan 11, 2025

Thanks, we managed to reproduce the issue with those steps. I have a few questions though:

  1. What would be a case where the central user has enough permissions to create users and create RLS policies, but not bypass them? If you can grant the former two, I imagine you could just use BYPASSRLS? The only case where I see this not being possible is if you're using RLS yourself for other things and explicitly don't want to use BYPASSRLS.
  2. Instead of making two separate policies, we could just remove FORCE ROW LEVEL SECURITY which should still enforce RLS for the tenant user, but not the central user as it's the table owner.

@stancl stancl marked this pull request as draft January 11, 2025 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants